Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow parsing rectypes in components #1764

Merged
merged 7 commits into from
Sep 10, 2024

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Sep 5, 2024

This change supports #392, which adds a way to use GC's rectypes as type definitions in components. Previously, only function types were supported and there was no way express array and struct types. This keeps the previous function decoding support based on peeking the function type 0x60 prefix but adds support for encoding rectypes with a new 0x00 prefix.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For added context this is an implementation of WebAssembly/component-model#392 but will wait on the exact path forward there to be settled on before landing here to avoid conflicting implementation/spec.

crates/wasm-encoder/src/reencode/component.rs Outdated Show resolved Hide resolved
crates/wasmparser/src/validator/types.rs Outdated Show resolved Hide resolved
crates/wasmparser/src/validator/types.rs Outdated Show resolved Hide resolved
crates/wast/src/component/binary.rs Outdated Show resolved Hide resolved
crates/wast/src/component/binary.rs Outdated Show resolved Hide resolved
crates/wast/src/core/types.rs Outdated Show resolved Hide resolved
@abrown abrown marked this pull request as ready for review September 9, 2024 17:05
@alexcrichton
Copy link
Member

Oh oops apologies but this as of now has a merge conflict in tests/roundtrip.rs, but I suspect that's not too hard to resolve. I've put #1765 in the merge queue though which, if merged by when you read this, should probably also cause some conflicts. Let me know though if anything is non-obvious how to resolve.

abrown and others added 7 commits September 10, 2024 09:58
This change supports [bytecodealliance#392], which adds a way to use GC's `rectypes` as
type definitions in components. Previously, only function types were
supported and there was no way express array and struct types. This
keeps the previous function decoding support based on peeking the
function type `0x60` prefix but adds support for encoding `rectypes`
with a new `0x00` prefix.

[bytecodealliance#392]: WebAssembly/component-model#392

Co-authored-by: Alex Crichton <[email protected]>
This follows along with the most recent discussion in the component
model PR ([bytecodealliance#392]).

[bytecodealliance#392]: WebAssembly/component-model#392

Co-authored-by: Alex Crichton <[email protected]>
@alexcrichton alexcrichton added this pull request to the merge queue Sep 10, 2024
Merged via the queue into bytecodealliance:main with commit 5ee4030 Sep 10, 2024
30 checks passed
@abrown abrown deleted the rectypes-rebased branch September 10, 2024 17:39
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Sep 20, 2024
This commit builds on the work of bytecodealliance#1764 to support the text format for
GC types in components more than before. This notably bring support for:

* `(sub ...)` types in components and module types
* `(rec ...)` groups in components and module types
* types can refer to themselves like with core wasm

The main consequence of this work is that unlike most other `$foo`
identifiers in the component model the identifiers found in types will
not automatically inject outer aliases to refer to outer types. For
example this will not parse:

    (component $C
      (type $t (struct))
      (component
        (type (array (ref $t)))
      )
    )

The reason for this is that automatic injection of an outer alias
requires that types are resolved and then their names are registered.
The resolution process queues up aliases to inject which are accounted
for during registration when indices are assigned. Here though because
types can refer to themselves (or future types in `rec` groups) the
registration process has to happen first before resolution. This means
that if resolution were to inject more type indices then that would mess
up the indexes already assigned.

This is hopefully relatively minor in terms of how often this'll bite
someone. For now various changes have been made to the name resolution
pass of components to handle this and some tests have been added too for
both positive and negative situations.
github-merge-queue bot pushed a commit that referenced this pull request Sep 20, 2024
This commit builds on the work of #1764 to support the text format for
GC types in components more than before. This notably bring support for:

* `(sub ...)` types in components and module types
* `(rec ...)` groups in components and module types
* types can refer to themselves like with core wasm

The main consequence of this work is that unlike most other `$foo`
identifiers in the component model the identifiers found in types will
not automatically inject outer aliases to refer to outer types. For
example this will not parse:

    (component $C
      (type $t (struct))
      (component
        (type (array (ref $t)))
      )
    )

The reason for this is that automatic injection of an outer alias
requires that types are resolved and then their names are registered.
The resolution process queues up aliases to inject which are accounted
for during registration when indices are assigned. Here though because
types can refer to themselves (or future types in `rec` groups) the
registration process has to happen first before resolution. This means
that if resolution were to inject more type indices then that would mess
up the indexes already assigned.

This is hopefully relatively minor in terms of how often this'll bite
someone. For now various changes have been made to the name resolution
pass of components to handle this and some tests have been added too for
both positive and negative situations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants